Skip to content

[#710] Fix replication catch-up re-sending updates with the original assured flag#714

Open
vharseko wants to merge 4 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix/issue-710-assured-catchup
Open

[#710] Fix replication catch-up re-sending updates with the original assured flag#714
vharseko wants to merge 4 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix/issue-710-assured-catchup

Conversation

@vharseko

@vharseko vharseko commented Jul 6, 2026

Copy link
Copy Markdown
Member

Fixes #710.

Problem

When a peer (RS or DS) is not "following" — its ServerWriter is catching up
from the changelog DB rather than being served from the in-memory queue —
updates were delivered with the assured flag, mode and safe-data level of
the original sender
.

ReplicationServerDomain.put() persists the original wire bytes via
publishUpdateMsg() before building the NotAssuredUpdateMsg, and the
substitution that strips the assured flag for non-eligible peers only happened
on the in-memory queue path (addUpdate()). MessageHandler.getNextMessage()
in the not-following branch re-reads the update from the changelog
(fillLateQueue()) and ServerWriter published it as-is. Every peer starts in
catch-up mode right after the handshake (following = false), so even a
just-connected peer receives its first updates through the changelog path.

Impact

  • A peer that must never be asked for an ack (RS from another group, or any RS
    once a safe-data level 1 update is satisfied) received the update with
    assuredFlag: true and replied a spurious AckMsg, skewing assured
    monitoring counters and silently violating the "assured does not cross group
    boundaries" contract.
  • New finding while fixing: when the spurious ack arrives while the ack
    window is still open (very likely, since catch-up delivery happens ms after
    publish), ExpectedAcksInfo.processReceivedAck() auto-unboxes a missing map
    entry (boolean = map.get(unknownId)) and throws a NullPointerException.
    It propagates out of ServerReader's loop, closing the connection to the
    acking peer — which reconnects, is in catch-up again, and can flap under
    steady assured traffic.
  • This made AssuredReplicationServerTest.testSafeDataLevelOne and
    testSafeReadOneRSComplex fail stochastically on CI (see Enable the remaining slow-group tests in the default build #702, worked around
    there by waiting until all peers are following).

Fix

Two independent parts:

  1. Normalize on both delivery paths. Move the substitution into
    ServerHandler.take(), the single point feeding ServerWriter from both the
    queue and the catch-up path. The assured flag is kept only while the ack
    window is still open and this server was eligible for an ack when the
    update was received (ReplicationServerDomain.isExpectedAck(csn, serverId)
    consults the live waitingAcks entry). This also resolves the corner case
    raised in the issue: a peer already in expectedServers keeps assured;
    everyone else (different group, connected after put(), expired window) gets
    a NotAssuredUpdateMsg. Assured counter skew is fixed as a side effect.
  2. Harden processReceivedAck() in both SafeReadExpectedAcksInfo and
    SafeDataExpectedAcksInfo to ignore acks from servers that are not expected
    instead of raising a NullPointerException.

Tests

  • ExpectedAcksInfoTest (new, precommit) — unit tests that an ack from a
    not-expected server id is ignored (no NPE) for both safe-read and safe-data.
  • AssuredReplicationServerTest.testCatchUpClearsAssuredFlag (new, precommit) —
    deterministic regression test: an assured update is published to the
    changelog before a fake RS connects, so it is served through the catch-up
    path and must arrive non-assured. Verified this test fails without the fix
    (received update assured flag is wrong ... assuredFlag: true) and passes
    with it.
  • Existing testSafeDataLevelOne/High/SafeReadOneRSComplex precommit tests stay
    green (queue path still keeps assured for eligible peers). 23 test runs, 0
    failures.

… flag (OpenIdentityPlatform#710)

A peer catching up from the changelog DB (following=false, the default right
after handshake) re-read updates via fillLateQueue() and received them with
the assured flag, mode and safe-data level of the original sender: the
NotAssuredUpdateMsg substitution was only applied on the in-memory queue path
in ReplicationServerDomain.addUpdate(). Non-eligible peers then replied a
spurious AckMsg; for a safe-read cross-group RS whose ack arrived while the
ack window was still open, processReceivedAck() auto-unboxed a missing map
entry and threw a NullPointerException that closed the peer connection.

Normalize updates in ServerHandler.take(), the single point feeding the
ServerWriter from both the queue and the catch-up path: keep the assured flag
only while the ack window is still open and this server was eligible for an
ack when the update was received (ReplicationServerDomain.isExpectedAck),
otherwise substitute a NotAssuredUpdateMsg. Harden both processReceivedAck()
implementations to ignore acks from servers that are not expected instead of
raising a NullPointerException.

Add ExpectedAcksInfoTest for the not-expected-ack case and
AssuredReplicationServerTest.testCatchUpClearsAssuredFlag, a deterministic
regression test that forces the catch-up path.
…n, lock-free ack membership, wider catch-up test coverage

- Move the assured-flag decision in ServerHandler.take() after
  acquirePermitInSendWindow(), so isExpectedAck() reflects the state at send
  time rather than at dequeue time (the send window may block longer than the
  assured timeout).
- ExpectedAcksInfo.isExpectedServer() now reads an immutable snapshot of the
  expected server ids taken at construction, instead of reading the
  lock-protected, value-mutated expectedServersAckStatus map without a lock.
- Parameterize testCatchUpClearsAssuredFlag over safe data level 1, safe data
  level > 1 and safe read; replace fixed sleeps with polling on changelog
  persistence and on received updates.
- Document that take() is the single assured guard complementing the queue-path
  substitution in addUpdate(), and that the UnsupportedEncodingException branch
  deliberately fails open to preserve replication delivery.
@vharseko vharseko changed the title Fix replication catch-up re-sending updates with the original assured flag (#710) [#710] Fix replication catch-up re-sending updates with the original assured flag Jul 6, 2026
@vharseko vharseko requested a review from maximthomas July 6, 2026 10:01
OpenIdentityPlatform#714)

nReceivedUpdates is incremented by the listen thread and polled from the test
thread in waitForReceivedUpdates(); mark it (and everyUpdatesAreOk, written
before the increment) volatile so the poll observes updates reliably.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replication server re-sends updates from changelog catch-up with the original assured flag

2 participants